Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move time series preparation for cosinor analysis to new function #913

Merged
merged 62 commits into from
Oct 11, 2023

Conversation

vincentvanhees
Copy link
Member

  • In preparation for work on Add optional cosinor analysis to part 6 #708 to include cosinor analysis in part 6 this PR moves code used for preparing the time series for cosinor analysis to its own function. This to help avoid duplication of code when also applying the analysis in part 6. No new unit tests because this should be covered by current unit test.
  • Added R2 as extra cosinor analysis outcome.
  • Minor effort to tidy up syntax in a few other functions such as g.part4.

Checklist before merging:

  • Existing tests still work (check by running the test suite, e.g. from RStudio).
  • Added tests (if you added functionality) or fixed existing test (if you fixed a bug).
  • Updated or expanded the documentation.
  • Updated release notes in inst/NEWS.Rd with a user-readable summary. Please, include references to relevant issues or PR discussions.
  • Added your name to the contributors lists in the DESCRIPTION and CITATION.cff files.

…R to a seperate function to ease re-using it in g.part5 #708
…works with both 5 second and 60second aggregated epochs
…be applied, and adding division by 1000 because analyses expect g-units #708
…R to a seperate function to ease re-using it in g.part5 #708
…works with both 5 second and 60second aggregated epochs
…be applied, and adding division by 1000 because analyses expect g-units #708
@vincentvanhees vincentvanhees marked this pull request as ready for review September 26, 2023 13:34
@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2023

Codecov Report

Merging #913 (75e6348) into master (d94ee28) will increase coverage by 0.03%.
The diff coverage is 89.13%.

❗ Current head 75e6348 differs from pull request most recent head 3db848b. Consider uploading reports for the commit 3db848b to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master     #913      +/-   ##
==========================================
+ Coverage   78.12%   78.16%   +0.03%     
==========================================
  Files          92       93       +1     
  Lines       12308    12323      +15     
==========================================
+ Hits         9616     9632      +16     
+ Misses       2692     2691       -1     
Files Coverage Δ
R/cosinorAnalyses.R 100.00% <100.00%> (ø)
R/g.analyse.avday.R 93.10% <100.00%> (+3.67%) ⬆️
R/g.analyse.perfile.R 84.23% <100.00%> (+0.18%) ⬆️
R/g.report.part5.R 85.24% <100.00%> (-0.04%) ⬇️
R/g.report.part4.R 83.58% <91.66%> (+0.16%) ⬆️
R/g.part4.R 80.41% <75.00%> (+0.17%) ⬆️
R/applyCosinorAnalyses.R 83.33% <83.33%> (ø)

Copy link
Collaborator

@jhmigueles jhmigueles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have already revised the changes and confirmed that all cosinor-related variables are exported in part 2. Tests are working and I don't see the need for new tests, maybe only for the calculation of the R2 if you think that is relevant.

Apart from that, only one minor comment, the documentation for cosinor in man/GGIR.Rd states that the argument is deprecated after version 1.5-24, but this is not correct, right? This argument is still used in GGIR to trigger the cosinor analyses.

@vincentvanhees
Copy link
Member Author

Apart from that, only one minor comment, the documentation for cosinor in man/GGIR.Rd states that the argument is deprecated after version 1.5-24, b

Well spotted, I think I accidentally copied this line from the parameter above. Will fix it now.

@vincentvanhees vincentvanhees merged commit 0e67881 into master Oct 11, 2023
8 checks passed
@vincentvanhees vincentvanhees deleted the issue708_centralise_cosinor branch October 12, 2023 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants